Cancel remaining fields on exceptions #238
Conversation
|
Thank you @mgorven - will have a look later this week or on the weekend. |
CodSpeed Performance ReportMerging #238 will not alter performanceComparing Summary
|
mgorven
force-pushed
the
cancel-tasks
branch
2 times, most recently
from
322f5f6 to
6456782
Compare
| for field in awaitable_fields: | ||
| tasks[create_task(results[field])] = field # type: ignore[arg-type] | ||
|
|
||
| done, pending = await wait(tasks, return_when=FIRST_EXCEPTION) |
There was a problem hiding this comment.
The other approach is to allow all fields to finish executing and then raise the exception.
| for task in pending: | ||
| task.cancel() | ||
|
|
||
| results.update((tasks[task], task.result()) for task in done) |
There was a problem hiding this comment.
This maintains the previous behaviour of not setting any results if an exception is raised, but I could change this to set the results we do have and then raise the exception.
There was a problem hiding this comment.
Does this behave in-line with the spec regarding partial results?
https://spec.graphql.org/draft/#sec-Handling-Execution-Errors
There was a problem hiding this comment.
I think this is fine because the potentially completed fields are siblings of the field with an exception, and the exception is propagated to the parent which will either be null or propagate further up. In either case these siblings field won't be present in the response.
gather() returns when the first exception is raised, but does not cancel any remaining tasks. These continue to run which is inefficient, and can also cause problems if they access shared resources like database connections. Fixes: graphql-python#236
|
Check is failing because coverage is not 100%, but this is because |
gather() returns when the first exception is raised, but does not cancel any remaining tasks. These continue to run which is inefficient, and can also cause problems if they access shared resources like database connections.
Fixes: #236